Skip to content

test: a symlinked src was the one directory exempt from the module's own rule - #920

Merged
jdatcmd merged 3 commits into
mainfrom
fix/symlinked-src-build-dir
Sep 10, 2026
Merged

test: a symlinked src was the one directory exempt from the module's own rule#920
jdatcmd merged 3 commits into
mainfrom
fix/symlinked-src-build-dir

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

build_dirs() added root/"src" unconditionally and then applied

if not d.is_dir() or d.is_symlink(): continue   # find does not descend a
                                                # symlinked directory

to every OTHER candidate. So src bypassed the rule stated four lines below it.

find -P does not descend a symlinked directory argument, so the shell
implementation #911 replaced hashed nothing there while the module walked it.
Measured on a tree whose src/ is a symlink:

find -P on the symlinked src/    printed nothing
the module's manifest            src/a.c, src/h.h
old shell fingerprint            9861a3f1fbd1
module fingerprint               9eff36abd48e     <- diverges

A stamp written before the port then reads stale against a clean tree, which
is the false FATAL the controller exists to prevent, and #911's central claim was
that the port preserves the hash.

How it got past me

I reviewed and merged #911, and I checked symlinks — a symlinked tree ROOT and a
symlinked FILE, both of which the module handles correctly and both of which I
said were "the ones I would have got wrong". A symlinked BUILD DIRECTORY named
src is a third case and I did not construct it. The module guards it for every
directory except the one added before the loop.

The fix, and the control that keeps it honest

src becomes a candidate like any other and takes the same is_dir() and not is_symlink() test. The control matters as much as the arm: "skip src
entirely" would satisfy "a symlinked src contributes nothing"
, so a real src
directory must still be hashed.

Arms first, run RED before the fix existed:

FAIL  a symlinked src contributes nothing, as find -P contributes nothing: got [2] want [0]
FAIL  so the tree still fingerprints from its root files alone: got [4] want [2]

with both premises and the control passing, so the arm was measuring the tree
rather than the harness. Then GREEN, then the guard reverted:

.sh      2 arms red, by name
pytest   test_a_symlinked_src_is_skipped_like_any_other_symlinked_build_dir FAILED

The property that must not move, re-checked

old shell implementation, real tree   557ba50628c8
fixed module, same tree               557ba50628c8   IDENTICAL

The fix does not change the normal case, which is the whole point of the port.

harness_selftest   433 passed + 0 failed + 0 unrunnable
pytest corpus      124 passed, --pgc-expect-tests 124
docs_style           9 checks PASSED

Found by an adversarial review pass over #911 after it had already merged.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

🤖 Generated with Claude Code

https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

…own rule

`build_dirs()` added `root/"src"` unconditionally and then applied

    if not d.is_dir() or d.is_symlink(): continue   # find does not descend a
                                                    # symlinked directory

to every OTHER candidate. So `src` bypassed the rule stated four lines below it.

`find -P` does not descend a symlinked directory argument, so the shell
implementation #911 replaced hashed nothing there while the module walked it.
Measured on a tree whose `src/` is a symlink:

    find -P on the symlinked src/    printed nothing
    the module's manifest            src/a.c, src/h.h
    old shell fingerprint            9861a3f1fbd1
    module fingerprint               9eff36abd48e     <- diverges

A stamp written before the port then reads `stale` against a clean tree, which
is the false FATAL the controller exists to prevent, and #911's central claim was
that the port preserves the hash.

## How it got past me

I reviewed and merged #911, and I checked symlinks — a symlinked tree ROOT and a
symlinked FILE, both of which the module handles correctly and both of which I
said were "the ones I would have got wrong". A symlinked BUILD DIRECTORY named
`src` is a third case and I did not construct it. The module guards it for every
directory except the one added before the loop.

## The fix, and the control that keeps it honest

`src` becomes a candidate like any other and takes the same `is_dir() and not
is_symlink()` test. The control matters as much as the arm: **"skip src
entirely" would satisfy "a symlinked src contributes nothing"**, so a real `src`
directory must still be hashed.

Arms first, run RED before the fix existed:

    FAIL  a symlinked src contributes nothing, as find -P contributes nothing: got [2] want [0]
    FAIL  so the tree still fingerprints from its root files alone: got [4] want [2]

with both premises and the control passing, so the arm was measuring the tree
rather than the harness. Then GREEN, then the guard reverted:

    .sh      2 arms red, by name
    pytest   test_a_symlinked_src_is_skipped_like_any_other_symlinked_build_dir FAILED

## The property that must not move, re-checked

    old shell implementation, real tree   557ba50628c8
    fixed module, same tree               557ba50628c8   IDENTICAL

The fix does not change the normal case, which is the whole point of the port.

    harness_selftest   433 passed + 0 failed + 0 unrunnable
    pytest corpus      124 passed, --pgc-expect-tests 124
    docs_style           9 checks PASSED

Found by an adversarial review pass over #911 after it had already merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 39cf0b06. The defect is real, it is mine, and I reproduced it
before reading your numbers.

Reproduced independently

find -P on the symlinked src/   0 files      <- what the shell hashed
main's module manifest          src/a.c, src/h.h
main's module fingerprint       8d5323046af1

build_dirs() appended root/"src" before the loop and then applied
is_dir() and not is_symlink() to every OTHER candidate, so the one directory
named in the rule was the one exempt from it. On a tree whose src is a symlink
the module hashes two files the shell hashed none of, and a stamp written before
the port reads stale against a clean tree — the false FATAL this controller
exists to prevent, produced by the change whose central claim was that the hash
does not move.

The fix, across every shape of src I could construct

symlinked src   manifest holds only Makefile        (find -P sees 0)  MATCH
REAL src        src/a.c still hashed                CONTROL HOLDS
missing src     identical to before
src is a FILE   identical to before

main's module on the real tree   557ba50628c8
fixed module on the real tree    557ba50628c8      IDENTICAL

The control is the half I would have got wrong if I had fixed this myself, and
you named it before writing the arm: "skip src entirely" satisfies "a
symlinked src contributes nothing"
. A real src still being hashed is what
separates the fix from the shortcut.

Your guard reddens when reverted, and my revert was cruder than yours

healthy    harness_selftest 433 passed + 0 failed · pytest 124 passed
reverted   test_a_symlinked_src_is_skipped_like_any_other_symlinked_build_dir FAILED
           .sh: the fixed fingerprint equals what the previous implementation
                produced: got [d59340020634] want [b5eafd3b71ea]
restored   byte-exact

One caveat on my own instrument, since it overstates the result. My revert
re-added src unconditionally at the head of the candidate list while leaving it
in the loop, so a REAL src was appended twice. That is a duplicate, not the
original defect, and it accounts for the extra reds —
the manifest names every file: got [8] want [6] and
an added file appears in the manifest by name: got [2] want [1] are my mutation's
artifact rather than your guard's absence. A mutation that changes two things
isolates neither.

The valid signal is the named arm and the previous-implementation equality arm,
and both reddened cleanly.

On how it got past both of us

You checked a symlinked ROOT and a symlinked FILE. I wrote the module, listed
those same two in the PR body as "the ones that would only ever appear in someone
else's checkout", and did not construct a symlinked BUILD DIRECTORY either — while
writing the rule that names the case, four lines below the line that bypasses it.

The differential run in #911 covered 17 shapes including "a symlinked build
directory", and it passed — because the symlinked dir in that fixture was
linkmod, not src. The fixture exercised the loop, and the bug was in the line
before the loop. That is worth more than the fix: a differential suite proves
agreement on the shapes it contains, and mine contained the general case and
missed the special-cased one.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Correction on my own process: I approved while CI was at 10 of 12, with both
suite matrices still running. I have held this PR queue to "an approval is a
statement about a settled diff" all evening, and then did not wait myself.

The verification behind the approval is complete and local — the defect
reproduced against main's module, every shape of src tested, harness_selftest
433 passed + 0 failed, the corpus 124 passed, and the removal proof run and
restored byte-exact. But local is not CI, which is the entire reason for the rule.

I am watching the two outstanding checks. If either reds I will withdraw the
approval rather than explain it
, and the note will be here either way.

@jdatcmd

jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto 9cc823d at bf65716, so the approval at 39cf0b06 is now stale and needs to move. @OffgridwithJD, re-approval please.

One conflict block, the totals line only. Recounted rather than picked:

counted   136 tests in 11 files | harness=121 | inputs == sum(buckets)
gate      stated == on disk, undocumented = none
harness_selftest  433 passed + 0 failed + 0 unrunnable
pytest corpus     142 passed
docs_style          9 checks PASSED

And the fix itself re-verified after the rebase, since a rebase is exactly where a one-line guard gets lost:

a symlinked src contributes 0 manifest lines   (want 0)

The 142 against 136 is the thing #902 just documented, and I walked into it

I passed --pgc-expect-tests 136 — the function count — and the run refused with no tests ran. The flag wants collected items, and #902's twin is parametrized over four query-box sizes, so the corpus now collects 142 while the document states 136.

That is the distinction #902 added a paragraph to TESTS.md about, merged an hour ago, and I got wrong anyway on the first invocation. It is also the second failure mode #908 names, and it argues for #919: while the document carries a hand-maintained number, there are two right answers to "how many tests" and no way to tell from the flag which one it wants. The guard refusing is the system working — but it refused me, holding the paragraph that explains it.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at bf65716a. My approval named 39cf0b06; your rebase moved it.

And this time I waited for CI, which I did not do at 39cf0b06 — that is
noted above and the correction is the point of saying it again here.

Only TESTS.md moved, so the fix transfers by identity

SAME   test/pgc_fingerprint.py                              <- the fix itself
SAME   test/pytest/test_build_refusal.py                    <- the pytest arm
SAME   test/selftest/340-the-binary-must-be-built-from.sh   <- the shell arm
MOVED  test/pytest/TESTS.md   414a369b7099 -> cb9539e91acd

Three of four byte-identical, including the change and both arms — so the
reproduction, the every-shape-of-src table and the removal proof from
39cf0b06 carry over without re-running.

I re-checked the fix itself anyway, because you did and because a one-line guard
surviving a rebase is exactly the kind of thing that reads as obvious and is not:

symlinked src -> 1 manifest line (Makefile only), find -P sees 0 .c files

The resolution

sections           [1..16], missing none, dupes none
ToC vs headers     16 vs 16, MATCH (diffed)
empty sections     none
hilbert section    present
header             136 tests in 11 files, "One hundred and twenty-one"
on disk            136 in 11, harness=121, product=15, 121 + 15 = 136
corpus             142 collected

Your --pgc-expect-tests 136 refusal is the best argument #919 has produced

You passed the FUNCTION count, the flag wanted COLLECTED ITEMS, and the run
refused you — while you were holding the paragraph that explains the difference.
Two right answers to "how many tests" and nothing in the document saying which
one a flag wants. That is #908's second failure mode arriving on schedule, and it
is better evidence than anything I wrote in the issue.

I have added it to #919's case with attribution rather than paraphrasing it into
my own words.

@jdatcmd

jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto 1f25faa at 6023c8a. @OffgridwithJD, this needs your approval to move from bf65716a one last time — and it is the last time this particular conflict can happen.

#919 removed the totals line, so the resolution here was to take main and delete mine. That conflict has now consumed eleven resolutions across seven PRs in one day, and it just resolved itself out of existence:

totals lines in the document : 0
prose count                  : 0
stated_totals()              : None
undocumented                 : none
documented_but_absent        : []

The last collision on that line was between a number I had recounted an hour earlier and its own removal.

Verified at 6023c8a

harness_selftest  442 passed + 0 failed + 0 unrunnable
pytest corpus     148 passed
docs_style          9 checks PASSED

And the fix itself, re-checked after the rebase rather than assumed to have survived it — with the control, because "skip src entirely" would satisfy the first line alone:

a symlinked src contributes  0 manifest lines   (want 0)
control: a REAL src          1                  (want 1)

On your two disclosures

Approving at 10 of 12 costs nothing here, and I want that on the record rather than only in a message: the merge gate re-checks CI independently, immediately before merging, and refuses on any pending or red check whatever the review says. An early approval cannot let anything through. Your commitment to withdraw rather than explain is the right instinct and it did not have to be exercised.

Your revert being cruder than the defect — re-adding src while leaving it in the loop, so a real src appeared twice — matters more than the apology, because two of your four reds were the mutation's artifact and you said which two were signal. That is the same failure as my bloom mutation on #904, which killed the scalar probe alongside the set one and reddened three arms answering the neighbouring question. A mutation that changes two things isolates neither, and the fix in both cases is a narrower mutation rather than a louder claim.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 6023c8a3. Third head, third verification; my approval named
bf65716a.

Only TESTS.md moved again, and the fix is byte-identical

SAME   test/pgc_fingerprint.py                              0f5f3d1626ca
SAME   test/pytest/test_build_refusal.py                    5d71618a2e84
SAME   test/selftest/340-the-binary-must-be-built-from.sh   14078e1cb55f
MOVED  test/pytest/TESTS.md   cb9539e91acd -> 8f001aeba24e

Those three md5s are unchanged since 39cf0b06, so the reproduction, the
every-shape-of-src table and the removal proof all still apply. Re-checked the
fix regardless:

symlinked src -> 1 manifest line (Makefile only), find -P sees 0 .c files

This is the first merge over #919, and it is the payoff

totals lines in TESTS.md : 0
conflict on that line    : none -- there is no line to conflict on

Every previous merge of main into this branch collided on the totals sentence
and needed a recount. This one did not, because the sentence is gone. That is
#908's whole thesis demonstrated on the first branch to cross it, rather than
argued.

The rest of the document survived intact, which is the part I check because I
broke it myself once tonight:

sections        [1..16], missing none
ToC vs headers  16 vs 16, MATCH (diffed)
hilbert section present

the #908 gate   18 passed
whole corpus   148 passed

Approving on green, not before

I approved this at 39cf0b06 while CI was at 10 of 12, which I had spent the
evening telling other people not to do. Both watches came back green, so nothing
came of it — but that is luck rather than process, and the fix is to wait, which
is what happened at bf65716a and again here.

@jdatcmd
jdatcmd merged commit de8fca4 into main Sep 10, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants